feat: LLM-27711 Codex /compact command#145
Conversation
NikolaiSviridov
force-pushed
the
nikolaisv/main/compact
branch
from
0719568 to
d51550a
Compare
| } | ||
|
|
||
| async tryHandle(prompt: acp.ContentBlock[], sessionState: SessionState): Promise<boolean> { | ||
| async tryHandle(prompt: acp.ContentBlock[], sessionState: SessionState): Promise<CommandHandlingResult | false> { |
There was a problem hiding this comment.
What is intention behind this return type? And why we added type type CommandHandlingResult = true | TurnCompletedNotification;?
|
|
||
| async runCompact(params: ThreadCompactStartParams): Promise<TurnCompletedNotification> { | ||
| let resolveTurnCompleted!: (event: TurnCompletedNotification) => void; | ||
| const turnCompleted = new Promise<TurnCompletedNotification>((resolve) => { |
There was a problem hiding this comment.
Why compact needs to wait for turn/end? Won't this be done automatically by common event handling?
| usage: this.buildPromptUsage(sessionState.lastTokenUsage), | ||
| _meta: this.buildQuotaMeta(sessionState), | ||
| }; | ||
| const interruptedResponse = await this.createInterruptedResponseIfNeeded(params.sessionId, turnCompleted, sessionState); |
There was a problem hiding this comment.
I would remain if (turnCompleted.turn.status === "interrupted") here and would remove "ifNeeded" from the extracted method. This would be more explicit and even more short.
| } | ||
|
|
||
| function createTurn(id: string, status: "inProgress" | "completed") { | ||
| function createTurn(id: string, status: TurnStatus) { |
There was a problem hiding this comment.
I would avoid propagating app-server API in the signature. This will cause app-server specifics to be added into CodexAcpServer, and will make maintaining more complicated.
| )).toBe(true); | ||
| }); | ||
|
|
||
| mockFixture.sendServerNotification(createTurnCompletedNotification("session-id", "compact-turn-id", "interrupted")); |
There was a problem hiding this comment.
why not AcpServer.cancel()?
| )).toBe(true); | ||
| }); | ||
|
|
||
| mockFixture.sendServerNotification(createTurnCompletedNotification("session-id", "compact-turn-id")); |
There was a problem hiding this comment.
Why we manually send turn end notification?
2 participants
No description provided.